Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce autoXXX keys for target auto-discovery #5335

Merged
merged 8 commits into from
Apr 21, 2018
Merged

Introduce autoXXX keys for target auto-discovery #5335

merged 8 commits into from
Apr 21, 2018

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Apr 10, 2018

In Rust 2015 absence of the configuration makes it default to not include auto-discovered targets (i.e false), with a warnings message.

In Rust 2018 absence makes it default to include auto-discovered targets (i.e true).

Fixes #5330


original

Fixes #5330

submitted for early review. feedback very welcome, I'm happy to iterate (and learn).

in particular I require assistance with the borrowing of warnings in clean_benches.

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@dwijnand dwijnand changed the title Introduce {target}.autodiscovery to opt out after 2018 Fixes #5330 Introduce {target}.autodiscovery to opt out after 2018 Apr 10, 2018
@dwijnand
Copy link
Member Author

oh, also I'm only testing examples at the moment. is that sufficient? if not how should I further test this?

@alexcrichton
Copy link
Member

Awesome, thanks for this! I think yeah we'll probably want tests for at least one type of target, but it's fine to not need tests for all of them.

I"m nto sure that autodiscovery is in the right location though as it's sort of a project-level distinction rather than a per-target distinction. Perhaps we could have keys like auto-examples = false inside the [package] section?

As we discussed on the other PR as well I think we'll want to start issuing warnings in the 2015 epoch if we'd like to message this out as part of the 2018 release

@dwijnand
Copy link
Member Author

I think yeah we'll probably want tests for at least one type of target, but it's fine to not need tests for all of them.

Do you mean you want one more? I've already got one test for the examples target.

I"m nto sure that autodiscovery is in the right location though as it's sort of a project-level distinction rather than a per-target distinction. Perhaps we could have keys like auto-examples = false inside the [package] section?

I'll give that a shot, see how it sits.

As we discussed on in other PR as well I think we'll want to start issuing warnings in the 2015 epoch if we'd like to message this out as part of the 2018 release

👍

Any tips to resolve my borrowchecker woes?

@alexcrichton
Copy link
Member

Oh oops sorry I meant at least two types of targets!

I'm on my phone but I'll check back for the borrow checker when I'm back at a computer

edition,
// FIXME: cannot borrow `*warnings` as mutable because previous closure requires unique access
// warnings,
&mut vec![],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could a temporary vector be allocated here and then appended to warnings outside the scope of the closure above?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! ideally I'd want the other way (e.g legacy_path_warnings), but I can't get it to work.

@bors
Copy link
Contributor

bors commented Apr 12, 2018

☔ The latest upstream changes (presumably #5348) made this pull request unmergeable. Please resolve the merge conflicts.

@dwijnand
Copy link
Member Author

Please resolve the merge conflicts.

by merging or rebasing?

@matklad
Copy link
Member

matklad commented Apr 12, 2018

@dwijnand I personally slightly prefer rebasing, but in general I don't think we care a lot about history :)

@bors
Copy link
Contributor

bors commented Apr 16, 2018

☔ The latest upstream changes (presumably #5360) made this pull request unmergeable. Please resolve the merge conflicts.

@dwijnand
Copy link
Member Author

this is ready for review.

I believe it now implements the desired behaviour discussed in #5330 (and here).

feedback always welcome :)

@dwijnand dwijnand changed the title Introduce {target}.autodiscovery to opt out after 2018 Introduce target auto-discovery options Apr 16, 2018
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking fantastic @dwijnand, thanks so much!

To summary to make sure I understand, we've got a bunch new keys under [package] like autobins = true which in 2018 default to true and in 2015 default to true if no [[bin]] is specified or false otherwise. In 2015 if autobins = false implicitly and we would have otherwise inferred a new target we issue a warning. Does that sound right?

Could you add some tests for autoXXX = false and the warning being disabled? Also, could the documentation in src/doc be updated with the new manifest keys?

"No auto-discovered {target_kind_human} targets included \
because at least one explicit [[{target_kind}]] section was added to Cargo.toml. \
Define the auto-discovery option for targets of this type, \
and beware that in Rust 2018 edition the default will flip to `true`.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this perhaps be reworded to:

An explicit [[{section}]] section is specified in Cargo.toml which currently disables Cargo from automatically inferring what other [[{section}]] targets would be. Disabling this inference behavior will change in the Rust 2018 edition and the following files will be included as a [[{section}]] target:

  • src/bin/foo.rs
  • src/bin/baz.rs

This is likely to break cargo build or cargo test as these files may not be ready to be compiled as a {{section}} target today. You can future-proof yourself and disable this warning by adding auto{{section}} = false to your [package] section. You may also move the files to a location where Cargo would not automatically infer them to be a target, such as in subfolders.

For more information on this warning you can consult https://github.com/rust-lang/cargo/issues/NUMBER

@@ -444,6 +444,7 @@ fn bench_with_lib_dep() {
name = "foo"
version = "0.0.1"
authors = []
autobins = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where autoXXX = true was added to the test suite could the files be reorganized a bit to not trigger the warning or need the key?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me have a look.

"#,
);

if cargotest::is_nightly() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This I believe can be avoided with masquerade_as_nightly_cargo() for Cargo, and there should be one #[test] function per test (so split this function into two).

This test may still need to be at the top of the function though because --edition doesn't exist on stable rustc, but it'll just be a prelude to all these tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

Copy link
Member Author

@dwijnand dwijnand Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually can you explain again, please? the only part I'm clear on is splitting the test.

what I'm testing here is:

  • in rust nightly + cargo edition feature + edition 2018 => the inferred examples/a.rs isn't dropped in the presence of a declared "do_magic" example
  • in rust stable (and therefore edition 2015) => the presence of a declared "do_magic" example, and the lack of an autoXXX, causes the inferred examples/a.rs to be dropped, therefore causing a warning and the cargo run --example a to fail

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sure yeah, so the way I see this test case is that there's actually two test cases internally and it's dynamically dispatched depending on rustc. I'd prefer to to split this out into two separate test cases where they may just be skipped entirely for nightly Rust

@dwijnand
Copy link
Member Author

thanks for the review @alexcrichton.

I've been working on this offline to get the test suite to pass - the commits I just pushed should CI go green now. I'm still stumbling my way to compiling and test-passing code, so please let me know if you spot any eye-soars or the like.

To summary to make sure I understand, we've got a bunch new keys under [package] like autobins = true which in 2018 default to true and in 2015 default to true if no [[bin]] is specified or false otherwise. In 2015 if autobins = false implicitly and we would have otherwise inferred a new target we issue a warning. Does that sound right?

it's hard to reason about this in terms of "default" because there are so many input variables. my attempt to summarise would be:

  • in 2015 you get a warning if you're dropping inferred targets
  • in 2018 you don't drop them, and therefore don't get a warning
  • with autoXXX you can specify whether to drop or not inferred targets (with false and true, respectively)

@alexcrichton
Copy link
Member

Ok sounds great!

@dwijnand
Copy link
Member Author

hey @alexcrichton, I think I've covered everything.

sorry for the force push but I think you'll find it easier to review this way (instead of the crazy git history tale of my progress to the end zone).

please let me know, happy to iterate with your feedback.

@dwijnand dwijnand changed the title Introduce target auto-discovery options Introduce autoXXX keys for target auto-discovery Apr 20, 2018
In Rust 2015 absence of the configuration makes it default to not
include auto-discovered targets (i.e false), with a warnings message.

In Rust 2018 absence makes it default to include auto-discovered
targets (i.e true).

Fixes #5330
@dwijnand
Copy link
Member Author

I thought I had understood that [/] is precisely to handle unix/windows paths, and yet...

  2 - |* "[..]foo[/]benches[/]bench_basic.rs"|
    + |* "C:\\projects\\cargo\\target\\cit\\t114\\foo\\benches\\bench_basic.rs"|
  2 - |* "[..]foo[/]examples[/]a.rs"|
    + |* "C:\\projects\\cargo\\target\\cit\\t952\\foo\\examples\\a.rs"|

what am I missing?

@alexcrichton
Copy link
Member

Hm, weird! I would have thought those would be working assertions as well... In any case it's fine to just use [..]bench_basic.rs as the assertion and not worry too much about whatever bug may be happening here

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again so much for working on this! Just a few small remaining nits

"\
An explicit [[{section}]] section is specified in Cargo.toml which currently disables Cargo from \
automatically inferring other {target_kind_human} targets. This inference behavior will change in \
the Rust 2018 edition and the following files will be included as a {target_kind_human} target:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using string literals for warnings in Cargo I've historically preferred to have manual line wrapping at 80 characters at that tends to be a good default at least for most terminals to display well. That'd just involve wrapping here along with removing the \ at the end of the lines

{rem_targets_str}
This is likely to break cargo build or cargo test as these files may not be ready to be compiled \
as a {target_kind_human} target today. You can future-proof yourself and disable this warning by \
adding {autodiscover_flag_name} = false to your [package] section. You may also move the files to \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the autofoo = false be surrounded by backticks here?

let mut rem_targets_str = String::new();
for t in rem_targets.iter() {
if let Some(p) = t.path.clone() {
rem_targets_str.push_str(&format!("* {:?}\n", p.0))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend using p.0.display() here instead fo {:?} to avoid the extraneous quotes

@alexcrichton
Copy link
Member

I've updated #5330 with the current state of this proposal as well. @rust-lang/cargo now would be a good time to review that issue's description and make sure you're ok with it!

@alexcrichton alexcrichton added the relnotes Release-note worthy label Apr 20, 2018
@dwijnand
Copy link
Member Author

that should be everything. thanks, the docs in #5330 look great.

do you think the docs for the manifest keys in reference guide are sufficient?


let targets = {
let mut legacy_bench_path = |bench: &TomlTarget| {
let legacy_path = package_root.join("src").join("bench.rs");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nice thing to do in the next PR would be to add if edition != Edition::Edition2015 { return None; } over here!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, this must be one of the instances of "backwards compatibility baggage" you originally referred to in #5330.

sure!

@alexcrichton
Copy link
Member

@dwijnand ah yeah the docs are good for now, we can always expand later if necessary!

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 21, 2018

📌 Commit ab5ac28 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Apr 21, 2018

⌛ Testing commit ab5ac28 with merge fd3be77...

bors added a commit that referenced this pull request Apr 21, 2018
Introduce autoXXX keys for target auto-discovery

In Rust 2015 absence of the configuration makes it default to not
include auto-discovered targets (i.e false), with a warnings message.

In Rust 2018 absence makes it default to include auto-discovered
targets (i.e true).

Fixes #5330

---

_original_

Fixes #5330

submitted for early review. feedback very welcome, I'm happy to iterate (and learn).

in particular I require assistance with the borrowing of `warnings` in `clean_benches`.
@bors
Copy link
Contributor

bors commented Apr 21, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing fd3be77 to master...

@bors bors merged commit ab5ac28 into rust-lang:master Apr 21, 2018
@dwijnand dwijnand deleted the target-autodiscovery branch April 21, 2018 18:12
@ehuss
Copy link
Contributor

ehuss commented Apr 21, 2018

Just FYI, building cargo with itself now displays this message. 😄

warning: An explicit [[bin]] section is specified in Cargo.toml which currently
disables Cargo from automatically inferring other binary targets.
This inference behavior will change in the Rust 2018 edition and the following
files will be included as a binary target:

* /Users/eric/Proj/rust/cargo/src/bin/cli.rs
* /Users/eric/Proj/rust/cargo/src/bin/command_prelude.rs

bors added a commit that referenced this pull request Apr 24, 2018
Drop legacy path support under Rust edition 2018 (or later)

builds on #5335

submitted for early feedback: wdyt @matklad? is this what you had in mind? what should change? what should be added? how should we test this? is the current (2015) messaging enough to drop it in 2018?

r? @matklad

<!--{"baseBranch":"rust-lang:cargo:target-autodiscovery"}-->
@ehuss ehuss added this to the 1.27.0 milestone Feb 6, 2022
bors added a commit that referenced this pull request Apr 29, 2024
fix(toml): Warn, rather than fail publish, if a target is excluded

### What does this PR try to resolve?

We have a couple of problems with publishing
- Inconsistent errors: if a target that `package` doesn't verify is missing `path`, it will error, while one with `path` won't, see #13456
- Users may want to exclude targets and their choices are
  - Go ahead and include them.  I originally excluded my examples before doc-scraping was a think.  The problem was if I had to set `required-features`, I then could no longer exclude them
  - Muck with `Cargo.toml` during publish and pass `--allow-dirty`

This fixes both by auto-stripping targets on publish.  We will warn the user that we did so.

This is a mostly-one-way door on behavior because we are turning an error case into a warning.
For the most part, I think this is the right thing to do.
My biggest regret is that the warning is only during `package`/`publish` as it will be too late to act on it and people who want to know will want to know when the problem is introduced.
The error is also very late in the process but at least its before a non-reversible action has been taken.
Dry-run and `yank` help.

Fixes #13456
Fixes #5806

### How should we test and review this PR?

Tests are added in the first commit and you can then follow the commits to see how the test output evolved.

The biggest risk factors for this change are
- If the target-stripping logic mis-identifies a path as excluded because of innocuous path differences (e.g. case)
- Setting a minimum MSRV for published packages: `auto*` were added in 1.27 (#5335) but were insta-stable.  `autobins = false` did nothing until 1.32 (#6329).  I have not checked to see how this behaves pre-1.32  or pre-1.27.  Since my memory of that error is vague, I believe it will either do a redundant discovery *or* it will implicitly skip discovery

Resolved risks
- #13729 ensured our generated target paths don't have `\` in them
- #13729 ensures the paths are normalize so the list of packaged paths

For case-insensitive filesystems, I added tests to show the original behavior (works locally but will fail when depended on from a case-sensitive filesystem) and tracked how that changed with this PR (on publish warn that those targets are stripped).  We could try to normalize the case but it will also follow symlinks and is likely indicative of larger casing problems that the user had.  Weighing how broken things are now , it didn't seem changing behavior on this would be too big of a deal.

We should do a Call for Testing when this hits nightly to have people to `cargo package` and look for targets exclusion warnings that don't make sense.

### Additional information

This builds on #13701 and the work before it.

By enumerating all targets in `Cargo.toml`, it makes it so rust-lang/crates.io#5882 and rust-lang/crates.io#814 can be implemented without any other filesystem interactions.

A follow up PR is need to make much of a difference in performance because we unconditionally walk the file system just in case `autodiscover != Some(false)` or a target is missing a `path`.

We cannot turn off auto-discovery of libs, so that will always be done for bin-only packages.
epage added a commit to epage/cargo that referenced this pull request Sep 24, 2024
PR rust-lang#5335 added `autobins`, etc for rust-lang#5330.  Nowhere in there is
discussion of `autolib`.

Cargo script disables support for additional build-targets by disabling
discovery.
Except we don't have a way to disable discovery of `autolib`, leading to rust-lang#14476.
By adding `autolib`, we can continue in that direction.

This also allows us to bypass inferring of libs on published packages,
like all other build-targets which were handled in rust-lang#13849.

As this seems fairly low controversy, this insta-stabilizes the field.
In prior versions of Cargo, users will get an "unused manifest key"
warning.
For packags where this is set by `cargo publish`, the warning will be suppressed and things will work as normal.
For `cargo vendor`, the same except there will be some churn in the
vendored source as this field will now be set.
For local development, it should be rare to set `autolib` so the lack of
error by discovering a file when this is set shouldn't be a problem.

Fixes rust-lang#14476
@epage epage mentioned this pull request Sep 24, 2024
epage added a commit to epage/cargo that referenced this pull request Sep 24, 2024
PR rust-lang#5335 added `autobins`, etc for rust-lang#5330.  Nowhere in there is
discussion of `autolib`.

Cargo script disables support for additional build-targets by disabling
discovery.
Except we don't have a way to disable discovery of `autolib`, leading to rust-lang#14476.
By adding `autolib`, we can continue in that direction.

This also allows us to bypass inferring of libs on published packages,
like all other build-targets which were handled in rust-lang#13849.

As this seems fairly low controversy, this insta-stabilizes the field.
In prior versions of Cargo, users will get an "unused manifest key"
warning.
For packags where this is set by `cargo publish`, the warning will be suppressed and things will work as normal.
For `cargo vendor`, the same except there will be some churn in the
vendored source as this field will now be set.
For local development, it should be rare to set `autolib` so the lack of
error by discovering a file when this is set shouldn't be a problem.

Fixes rust-lang#14476
epage added a commit to epage/cargo that referenced this pull request Sep 25, 2024
PR rust-lang#5335 added `autobins`, etc for rust-lang#5330.  Nowhere in there is
discussion of `autolib`.

Cargo script disables support for additional build-targets by disabling
discovery.
Except we don't have a way to disable discovery of `autolib`, leading to rust-lang#14476.
By adding `autolib`, we can continue in that direction.

This also allows us to bypass inferring of libs on published packages,
like all other build-targets which were handled in rust-lang#13849.

As this seems fairly low controversy, this insta-stabilizes the field.
In prior versions of Cargo, users will get an "unused manifest key"
warning.
For packags where this is set by `cargo publish`, the warning will be suppressed and things will work as normal.
For `cargo vendor`, the same except there will be some churn in the
vendored source as this field will now be set.
For local development, it should be rare to set `autolib` so the lack of
error by discovering a file when this is set shouldn't be a problem.

Fixes rust-lang#14476
epage added a commit to epage/cargo that referenced this pull request Sep 26, 2024
PR rust-lang#5335 added `autobins`, etc for rust-lang#5330.  Nowhere in there is
discussion of `autolib`.

Cargo script disables support for additional build-targets by disabling
discovery.
Except we don't have a way to disable discovery of `autolib`, leading to rust-lang#14476.
By adding `autolib`, we can continue in that direction.

This also allows us to bypass inferring of libs on published packages,
like all other build-targets which were handled in rust-lang#13849.

As this seems fairly low controversy, this insta-stabilizes the field.
In prior versions of Cargo, users will get an "unused manifest key"
warning.
For packags where this is set by `cargo publish`, the warning will be suppressed and things will work as normal.
For `cargo vendor`, the same except there will be some churn in the
vendored source as this field will now be set.
For local development, it should be rare to set `autolib` so the lack of
error by discovering a file when this is set shouldn't be a problem.

Fixes rust-lang#14476
epage added a commit to epage/cargo that referenced this pull request Sep 26, 2024
PR rust-lang#5335 added `autobins`, etc for rust-lang#5330.  Nowhere in there is
discussion of `autolib`.

Cargo script disables support for additional build-targets by disabling
discovery.
Except we don't have a way to disable discovery of `autolib`, leading to rust-lang#14476.
By adding `autolib`, we can continue in that direction.

This also allows us to bypass inferring of libs on published packages,
like all other build-targets which were handled in rust-lang#13849.

As this seems fairly low controversy, this insta-stabilizes the field.
In prior versions of Cargo, users will get an "unused manifest key"
warning.
For packags where this is set by `cargo publish`, the warning will be suppressed and things will work as normal.
For `cargo vendor`, the same except there will be some churn in the
vendored source as this field will now be set.
For local development, it should be rare to set `autolib` so the lack of
error by discovering a file when this is set shouldn't be a problem.

Fixes rust-lang#14476
bors added a commit that referenced this pull request Sep 27, 2024
feat(toml): Add `autolib`

### What does this PR try to resolve?

PR #5335 added `autobins`, etc for #5330.  Nowhere in there is
discussion of `autolib`.

Cargo script disables support for additional build-targets by disabling
discovery.
Except we don't have a way to disable discovery of `autolib`, leading to #14476.
By adding `autolib`, we can continue in that direction.

This also allows us to bypass inferring of libs on published packages,
like all other build-targets which were handled in #13849.

Fixes #14476

### How should we test and review this PR?

### Additional information

As this seems fairly low controversy, this insta-stabilizes the field.
In prior versions of Cargo, users will get an "unused manifest key"
warning.
For packages where this is set by `cargo publish`, the warning will be suppressed and things will work as normal.
For `cargo vendor`, the same except there will be some churn in the
vendored source as this field will now be set.
For local development, it should be rare to set `autolib` so the lack of
error by discovering a file when this is set shouldn't be a problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Release-note worthy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make explicit opt-out of target discovery neccessary for Rust 2018
6 participants